Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

App service support for MI #537

Merged
merged 25 commits into from
Jan 17, 2025
Merged

Conversation

4gust
Copy link
Collaborator

@4gust 4gust commented Nov 28, 2024

Added support for App service
Added support for logging in Managed Identity

@4gust 4gust marked this pull request as ready for review November 28, 2024 14:38
@4gust 4gust requested review from chlowell and bgavrilMS and removed request for rayluo and bgavrilMS November 28, 2024 14:38
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comments.

Copy link
Collaborator

@chlowell chlowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blockers are:

  • query parameter names (docs)
  • returned token expiration time is always 0
  • fake token responses in tests don't resemble actual responses

Expiration time is always 0 because App Service responses include only expires_on (docs) and MSAL unmarshals only expires_in, using a custom UnmarshalJSON to convert that duration to an instant. Unit tests pass despite this because their fake responses always include expires_in

apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Show resolved Hide resolved
apps/managedidentity/managedidentity.go Show resolved Hide resolved
apps/managedidentity/managedidentity.go Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
Updated the resource Id parameter for every source except IMDS
apps/managedidentity/managedidentity_test.go Show resolved Hide resolved
apps/internal/oauth/ops/accesstokens/tokens.go Outdated Show resolved Hide resolved
apps/internal/oauth/ops/accesstokens/tokens.go Outdated Show resolved Hide resolved
apps/internal/oauth/ops/accesstokens/tokens.go Outdated Show resolved Hide resolved
4gust added 2 commits January 3, 2025 10:01
Added support for expires_in
different time formats for expire_in
support for principle_id for app service in managed identity
apps/internal/json/types/time/time.go Outdated Show resolved Hide resolved
apps/internal/oauth/ops/accesstokens/tokens.go Outdated Show resolved Hide resolved
@AndyOHart AndyOHart self-requested a review January 9, 2025 15:44
Copy link
Collaborator

@AndyOHart AndyOHart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

payload: `
{
"access_token": "secret",
"expires_on": "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also have a success case in which only expires_in is present

apps/internal/oauth/ops/accesstokens/tokens.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
@4gust 4gust merged commit faf744d into andyohart/managed-identity Jan 17, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants